compiler: drop private from three module-crossing declarations#1376
Merged
Conversation
These are declared `private` inside their modules but referenced from outside them, so they were never actually private — module-level `private` is not enforced today (#1343), which is the only reason this compiles. Making them public just states what is already true, ahead of turning that enforcement on. - Config.Config (Config.sk): the shared config record, used as a type in ~20 backend modules (optimize, peephole, AsmOutput, compile, ...). - OuterIstToIR.makeOuterIst, AsmOutput.mangle: internal helpers the driver in compile.sk calls from global scope. (PrettyIR has its own private `mangle`; that one is same-module and stays private.) No behavioural change — the compiler rebuilds itself cleanly. Refs #1343. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mbouaziz
force-pushed
the
unprivate-mislabeled-decls
branch
from
July 20, 2026 11:53
d5cc14a to
e5d7122
Compare
mbouaziz
enabled auto-merge
July 20, 2026 11:53
mbouaziz
added a commit
that referenced
this pull request
Jul 21, 2026
…into privates (#1421) The SKStore side of unblocking module-`private` enforcement (#1343), following PR #1376 (the compiler-internal mislabels). SKJSON and SKDB reach across package boundaries into SKStore's memory-region and context-sync primitives, which are `private native fun`; this only compiles because module-`private` isn't enforced yet. ## What this does **`gContextSync` stays private, behind a new public `SKStore.syncContexts`.** This is the interesting one. `SQLContext._exec` in SKDB hand-rolled the whole commit path over its *own* `Contexts` — obstack, run `f`, `gContextSync`, notify, run actions, destroy. `syncContexts` is exactly that sequence (the counterpart of the existing `runWithResult` for callers that own a `Contexts` rather than driving the global registry), so `_exec` becomes a call and the last external use of `gContextSync` is gone. **`newObstack` / `destroyObstack` become public.** This is a change from the "keep everything private" framing, and I want to flag it rather than bury it — happy to go the other way if you'd prefer. The reasoning: - Their partner `destroyObstackWithValue` is *already* public, yet useless from outside, because the only way to obtain an `Obstack` to pass it was the private `newObstack`. The visibility split is incoherent — the same shape as `Config.Config` in #1376. - Three call sites cannot be expressed as a `withRegion*` lambda, because their control flow escapes the region: `SqlTailer`'s poll loop `break`s the loop from inside the region, and the two `main`s (`sql`, `skjson`) `skipExit` from inside a program-lifetime region. A `~>` lambda can carry neither a `break` nor a mid-body `skipExit` out. Forcing them would mean restructuring a tailer loop and two entry points — real behavioural risk for no real gain. So the public region API is `withRegion` / `withRegionVoid` / `withRegionValue` (preferred) plus the raw `newObstack` / `destroyObstack` / `destroyObstackWithValue` escape hatch, and `gContextSync` is fully internal. ## Opportunistic cleanup Four sites that *did* fit a lambda — SKJSON's `execGen`/`execCheck` and SKDB's two `@wasm_export` reactive-query functions — moved to `withRegionVoid`. Each previously had `destroyObstack()` *outside* its `try`, so the region leaked on an uncaught throw; the wrapper reclaims it on both paths. Left alone: the two `main`s, the tailer, and the prelude's own tests, which legitimately use the now-public raw primitives. ## Correctness The commit-path migration is the sensitive part. `syncContexts` reproduces `_exec`'s sequence exactly — same tick, same `gContextSync` arguments, same notify-then-actions order, same region reclaim on the exception path. The one deliberate difference: `_exec` reads `this.globals` into a local before the closure, because a frozen `~>` cannot capture `this`. ## Verification `skargo build --release --bin skdb` succeeds (~123s), which compiles `std`, `skjson`, `sqlparser` and `skdb` against these changes. That is what CI's `skdb` job runs. One caveat I'll be upfront about: I could not exercise the **dev**-profile build locally, because my installed `skargo` (0.3.5, from 5a9a737) predates the #1378 dispatch fix (#1380) and still hits *that* bug on `sql/` — unrelated to this change (it reproduces on a clean tree). CI's toolchain has #1380, so it will cover the dev path. I have **not** run SKDB's functional test suite (`make test-native`) locally — it needs a matched toolchain build I haven't done. The release build proves it compiles and links; it does not prove the commit path is behaviourally identical at runtime. Given this touches transaction commit, that's worth a maintainer eye or a green CI run before merge. — Claude Opus 4.8 (1M context)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prep for enforcing module-level
private(#1343). Three declarations are markedprivateinside their modules but referenced from outside them — so they were never actually private; module-privatesimply isn't enforced today, which is why this compiles at all. This makes them public to state what is already true, so that turning enforcement on later doesn't break the build here.Because the feature is unenforced, this is a no-op at runtime — the compiler rebuilds itself cleanly (verified: a full
make stage1from this source succeeds).Config.ConfigConfig.sk:43config: Config.Config)OuterIstToIR.makeOuterIstOuterIstToIR.sk:4884compile.sk:236(driver, global scope)AsmOutput.mangleAsmOutput.sk:1139compile.sk:618(driver, global scope)Config.Configis a plain mislabel — it's the compiler's shared config record, threaded as a type through the whole backend. The other two are internal helpers the driver legitimately calls. NotePrettyIRhas its ownprivate fun mangle(PrettyIR.sk:626); that one is same-module and stays private — onlyAsmOutput.mangleis the cross-module one.This is the first of the prep PRs from my analysis on #1343 (the harmless un-private migration). The obstack/
gContextSynccross-package reaches are handled separately, by giving SKJSON/SKDB sanctioned SKStore wrappers rather than un-privating the internals.— Claude Opus 4.8 (1M context)